Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix sync-pnpm when already synced previously #61

Merged
merged 1 commit into from
Feb 27, 2023
Merged

Conversation

simonihmig
Copy link
Contributor

The sync script would skip syncing the dist folder when it already exists. This is not good when running things locally, like when you run the test-app, and build the addon in watch mode, you would still need to run the sync script after changes in the addon manually. But without this fix here, it would not update the dist output with the addon changes.

Currently my development workflow with this is:

  • pnpm start (runs test-app)
  • in another terminal:
    • cd <addon-dir>
    • pnpm start (rollup in watch mode)
  • in yet another terminal:
    • cd test-app
    • on every change of the addon run manually: pnpm _syncPnpm

Far from good, but at least works...

@changeset-bot
Copy link

changeset-bot bot commented Feb 27, 2023

⚠️ No Changeset found

Latest commit: 64b0216

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@github-actions
Copy link
Contributor

Preview URLs

Env: preview
Docs: https://a7941215.ember-headless-form.pages.dev

@nicolechung
Copy link
Contributor

Suggestion (maybe I should file an issue for this): Can we change the port of either the docs-app or the test-app so I can run both at the same time?

@nicolechung
Copy link
Contributor

on every change of the addon run manually: pnpm _syncPnpm

Wait...what is the difference between running pnpm i -f and pnpm_syncPnpm between each build of the package?

Copy link
Contributor

@ynotdraw ynotdraw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! I also found success with using pnpm link, but not sure if it's any better DX wise. Here's what I found with ember-toucan-core:

# add-on
cd ember-toucan-core
pnpm link .

# go to docs-app now and link it
cd ../docs-app
pnpm link @crowdstrike/ember-toucan-core

Now build the add-on in watch mode

cd ember-toucan-core
pnpm start

Then in another terminal, start the docs app:

cd docs-app
pnpm start

Now if you make a change in ember-toucan-core, it auto-rebuilds and the updates are shown in the docs-app!


FWIW, I just tried this in a super small example, not with actually building anything real, so there may still be issues with link

@nicolechung
Copy link
Contributor

Screen Shot 2023-02-27 at 12 04 47 PM

I know that I should know this given I reviewed the earlier version, but how do I run syncPnpm

Attached is an image of what I have tried

@nicolechung
Copy link
Contributor

Screen Shot 2023-02-27 at 12 04 47 PM

I know that I should know this given I reviewed the earlier version, but how do I run syncPnpm

Attached is an image of what I have tried

NVM Tony just showed me its in the app directory...

@simonihmig
Copy link
Contributor Author

NVM Tony just showed me its in the app directory...

Yeah, it's only available to apps. Ideally we shouldn't call this ourselves of course (hence the underscored name), but we are still figuring things out. Seems @NullVoxPopuli is also up for something:

Copy link
Contributor

@nicolechung nicolechung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay now that I figured out how to get this running...this is definitely an improvement!

I only have to run the command pnpm _synchPnpm once and it works!

@simonihmig simonihmig merged commit b644d69 into main Feb 27, 2023
@simonihmig simonihmig deleted the fix-sync-pnpm branch February 27, 2023 17:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants